integration/testdata/cases/pattern_predicate_direction_inline.json (1)

1-131: ⚡ Quick win

Consider extracting common fixture data to reduce duplication.

All six test cases define nearly identical inline fixtures (nodes u1, u2, g1, g2, g3, g4 and their edges). While this makes each test self-contained, it creates a maintenance burden—any fixture adjustment requires updating six locations.

If these fixtures need to remain inline, consider at least documenting why the duplication is intentional (e.g., for regression test isolation).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@integration/testdata/cases/pattern_predicate_direction_inline.json` around
lines 1 - 131, The fixtures for the six cases (e.g., cases named "regression:
incoming negated pattern with contains predicate (left-directed form)",
"right-directed equivalent", etc.) duplicate the same nodes and edges; extract
that shared nodes/edges JSON into a single reusable fixture object (or helper
function) and reference it from each case to avoid repetition, or if inline
fixtures must remain, add a short explanatory comment in each case noting the
intentional duplication for regression/isolation; update the cases that
reference the repeated nodes (u1,u2,g1,g2,g3,g4) and edges (EdgeKind1/EdgeKind2
entries) to use the shared fixture identifier or include the comment.

cypher/models/pgsql/test/pattern_predicate_shape_test.go (2)

38-40: 💤 Low value

Add a comment explaining why this pattern is forbidden.

The forbidden fragment "from s0 join edge e0 on (s0.n0).id = e0.end_id" represents the buggy SQL structure that this fix addresses, but the reason isn't documented. A brief comment would help future maintainers understand the regression being prevented.

📝 Suggested comment

 	forbiddenFragments := []string{
+		// This join pattern caused incorrect correlation in negated pattern predicates (BED-6695).
+		// The fix restructures the CTE to avoid binding the outer reference too early.
 		"from s0 join edge e0 on (s0.n0).id = e0.end_id",
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cypher/models/pgsql/test/pattern_predicate_shape_test.go` around lines 38 -
40, Add a short explanatory comment above the forbiddenFragments entry for the
string "from s0 join edge e0 on (s0.n0).id = e0.end_id" stating that this SQL
pattern is forbidden because it uses a nested node accessor `(s0.n0).id` in the
JOIN ON clause (producing incorrect/buggy SQL generation or runtime errors for
nullable/composite node columns) and that the test prevents regressions related
to that specific buggy join shape; update the comment to reference the exact
fragment and the regression it guards against so future maintainers understand
why it must remain forbidden.

---

24-30: 💤 Low value

Consider deriving kind IDs dynamically from test constants.

The hard-coded array values [1] and [3] in the SQL assertions must match NodeKind1 and EdgeKind1 as mapped by newKindMapper(). If the kind mapper changes, these assertions could silently pass with incorrect SQL.

♻️ Alternative: compute expected values from the mapper

You could call newKindMapper() and extract the mapped IDs to build the expected fragments dynamically. However, since this is a focused regression test for a specific bug fix and the mapper is stable test infrastructure, the current approach may be acceptable.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cypher/models/pgsql/test/pattern_predicate_shape_test.go` around lines 24 -
30, The test currently hardcodes kind ID arrays in requiredFragments which can
drift from the mapping; obtain the mapper via newKindMapper(), use it to look up
the mapped IDs for NodeKind1 and EdgeKind1, and construct the expected SQL
fragments using those computed IDs instead of literal "[1]" and "[3]"; update
the requiredFragments construction (the variable named requiredFragments in the
test) to interpolate the derived values so assertions remain correct if
newKindMapper() changes.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes